-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add confidential prefix check utility #1059
Conversation
return false; | ||
} | ||
let prefix = &data[..PREFIX_LEN]; | ||
// u8 representation of "confidential" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we feel pretty good that this is otherwise going to be an invalid transaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. We need a better guarantee, preferably in the form of an EIP or some existing standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are known "reserve" or "invalid" evm byte code. can't we use those somehow creatively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a link I can read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namely:
For a single byte whose value is in the [0x00, 0x7f] range, that byte is its own RLP encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, does the data
blob include the CREATE/CALL instruction (e.g., 0xf0
or 0xf1
) or is the create/call implicit and only the initcode/sighash+abi is in the data blob?
If it is the latter this is more complicated as we need a value which at the same time:
- Is unlikely to be a sighash (I assume a 32-byte hash, e.g. Keccak?).
- Is unlikely to be initcode (EVM bytecode).
There are some opcodes which are invalid (e.g., based on this it seems like e.g. 0x0c
is an invalid opcode). But of course a future revision could add opcodes in there. We could use some combination which is unlikely, e.g. 0x000c0d636f...6c
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create/call is implicit based upon there existing a to address or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then something like some invalid bytecodes at the start and then some other specific value like Web3c-Confidential
should be fine? For initcode the invalid opcodes should never appear and for a hash a specific constant is very very unlikely (but possible - can be made more unlikely by being longer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like parity uses the fact that WASM modules prepend the 4 byte prefix \0asm
to know if it's dealing with WASM code. This prefix is assumed to be part of both the initcode and the bytecode of the contract. See https://github.com/oasislabs/parity/blob/ekiden/ethcore/src/factory.rs#L33.
For confidential contracts we can do something similar.
For CREATE
, say, in the web3c.js client, we can prepend \0pri
(or some other 4 byte prefix to be consistent with wasm's prefix) to the data
field in the transaction.
During the CREATE, in the runtime, we can check for the prefix, and when we finally store the bytecode of the contract (which drops the initcode), we can re-prepend the \0pri
to the stored byte code. On all subsequent CALL
s, we can do the check for the prefix by looking at the account's stored code, and decrypt the data field if its spotted.
This scheme is mainly different from the previous above conversation because, for CALLs, the data
field itself doesn't have a prefix and is just encrypted. As a result, we can remove the possibility of the sighash colliding with our prefix. Furthermore, the \0
byte represents the STOP opcode in the EVM, and so during CREATE, we don't have to worry about a collision with valid bytecode either, since that won't ever be the first instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documentation blocks for added functions and constants. Also to all the existing ones.
} | ||
let prefix = &data[..PREFIX_LEN]; | ||
// u8 representation of "confidential" | ||
let confidential_prefix = [99, 111, 110, 102, 105, 100, 101, 110, 116, 105, 97, 108]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to make this a module-level constant.
if data.len() < PREFIX_LEN { | ||
return data; | ||
} | ||
data[PREFIX_LEN..].to_vec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this check that data
actually has the prefix, instead of assuming that the first PREFIX_LEN
bytes are the prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the scheme we use. It should be fine to assume it’s the first couple of bytes. We just need to make sure the prefix chosen is not valid bytecode so that we don’t mistake a valid transaction for an encrypted one.
Is there a specific reason why we might want the prefix to be anywhere in the ‘data’ that I’m missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think @Yawning is saying that the prefix could be anywhere in the data, but that the function should maybe first check if data[..PREFIX_LEN] == CONFIDENTIAL_PREFIX
and not just assume that it is just based on length as it is currently.
Could return the data unchanged otherwise or even return an Option
in this case and return None
in case the prefix is not there. In any case this behavior should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see good point. I'll add the check. The assumption I should've stated was that is_encrypted
returned true.
Putting back into [WIP] while I address comments and update the |
With the new scheme, putting the confidential prefix onto the contract's bytecode, instead of onto the the transaction's data field, I'll be pushing these changes down into parity and so will not need this PR. Closing. |
No description provided.